-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add new site preference for indexing out-of-stock products #218
base: develop
Are you sure you want to change the base?
Conversation
- Added new site preference for enabling/disabling to index out of stock products - Wrote unit tests
…import in the test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are things missing:
- When we are indexing records with no attributes that are
computedFromBaseProduct
, we don't pass through thejobHelper
, but we generate the records one by one with this code: https://github.com/algolia/algoliasearch-sfcc-b2c/blob/24.6.0/cartridges/int_algolia/cartridge/scripts/algolia/steps/algoliaProductIndex.js#L303-L321. So the preference needs to be checked here too - The delta job should delete products when they become out of stock and the setting is disabled
Also, do we want to keep the in_stock
attribute when out-of-stock products are not indexed? It doesn't seem useful.
Otherwise, the tests are currently not testing anything.
cartridges/int_algolia/cartridge/scripts/algolia/model/algoliaLocalizedProduct.js
Outdated
Show resolved
Hide resolved
cartridges/bm_algolia/cartridge/templates/resources/algolia.properties
Outdated
Show resolved
Hide resolved
@@ -49,6 +49,7 @@ function handleSettings() { | |||
algoliaData.setPreference('EnableContentSearch', algoliaEnableContentSearch); | |||
algoliaData.setPreference('EnableRecommend', algoliaEnableRecommend); | |||
algoliaData.setPreference('EnablePricingLazyLoad', algoliaEnablePricingLazyLoad); | |||
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted); | |
algoliaData.setPreference('IndexOutOfStock', params.IndexOutOfStock.submitted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you reverted it back? In the commit message you say:
Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.
What are the potential issues since it's a new preference that we are adding?
const localizedProduct = new AlgoliaLocalizedProduct({ product, attributeList: ['objectID'] }); | ||
|
||
expect(localizedProduct.objectID).toBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? The test doesn't check anything regarding skipping out-of-stock products.
Here you are generating an AlgoliaProduct from a SFCC product, with the objectID
attribute. But objectID
doesn't exist in SFCC products, so it will necessarily be null 🤷
Please take the time to run step by step it in debug to understand what is happening.
const localizedProduct = new AlgoliaLocalizedProduct({ product, attributeList: ['objectID'] }); | ||
|
||
expect(localizedProduct).not.toBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, there is no relation with indexing out-of-stocks products at all.
You are instantiating a new AlgoliaLocalizedProduct
, so it's normal for it to not be null
Co-authored-by: Sylvain Bellone <[email protected]>
Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.
- Introduced a new function `isIncludeOutOfStock` to determine if out-of-stock products should be included based on the `InStockThreshold` and `IndexOutofStock` preferences. - Updated existing product filtering logic to utilize the new function, ensuring that products are only indexed if they meet the stock criteria. - Adjusted the `algoliaLocalizedProduct` model to reflect the new out-of-stock handling. - Modified indexing steps to handle the inclusion and exclusion of products based on their stock status. This change improves the accuracy of product indexing in Algolia, allowing for better control over which products are indexed based on their availability.
- Removed unnecessary imports and constants related to Algolia data preferences in `jobHelper.js`. - Simplified the calculation of index names in `algoliaProductDeltaIndex.js` by directly using the locale in the push operation. - Enhanced unit tests for `jobHelper` and `algoliaLocalizedProduct` to mock Algolia data preferences more effectively, ensuring better test isolation and coverage. - Updated snapshot tests to reflect changes in the indexing process.
- Updated the `isInclude` function in `productFilter.js` to enhance product filtering by adding comments for clarity and ensuring that option products are excluded from indexing. - Refactored the `process` function in `algoliaProductIndex.js` to streamline the return logic, improving readability. - Modified the test in `algoliaLocalizedProduct.test.js` to return `null` instead of an empty array, aligning with expected behavior for better test accuracy. These changes enhance the accuracy and maintainability of the Algolia product indexing process.
- Introduced a new mock variable `mockRecordModel` to manage the record model state in tests, replacing the previous global preference setting. - Updated test cases to utilize the new mock variable for setting the record model, ensuring consistency and clarity in test behavior. - This change enhances the maintainability of the test suite by isolating the record model configuration from global state, improving test reliability.
- Refactored tests in `algoliaLocalizedProduct.test.js` to improve clarity and maintainability. - Updated the `isIncludeOutOfStock` tests to use more descriptive test names and streamlined mock implementations. - Ensured that the tests accurately reflect the logic for including or excluding out-of-stock products based on the `IndexOutofStock` preference. - This change enhances the reliability of the test suite and ensures better coverage of product filtering scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When some tests fail, you need to understand the root cause before updating them.
To me changing the snapshot is not the correct fix here.
@@ -49,6 +49,7 @@ function handleSettings() { | |||
algoliaData.setPreference('EnableContentSearch', algoliaEnableContentSearch); | |||
algoliaData.setPreference('EnableRecommend', algoliaEnableRecommend); | |||
algoliaData.setPreference('EnablePricingLazyLoad', algoliaEnablePricingLazyLoad); | |||
algoliaData.setPreference('IndexOutofStock', params.IndexOutofStock.submitted); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you reverted it back? In the commit message you say:
Updated the preference key from 'IndexOutOfStock' to 'IndexOutofStock' in AlgoliaBM.js to ensure consistency and prevent potential issues in preference handling.
What are the potential issues since it's a new preference that we are adding?
@@ -239,427 +239,4 @@ exports[`process default 1`] = ` | |||
] | |||
`; | |||
|
|||
exports[`process master-level indexing 1`] = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such a big diff in tests result should draw your attention.
Basically the function was previously generating records and now return an empty array.
You have to understand why before updating the test to make it pass. Here, you are potentially hiding a bug!
The problem can just come from the mock, in which case you can update it, but from what I see, your implementation of isIncludeOutOfStock
differs from what you had initially done in algoliaLocalizedProduct.js
. Is it normal?
cartridges/int_algolia/cartridge/scripts/algolia/steps/algoliaProductDeltaIndex.js
Show resolved
Hide resolved
- Corrected the spelling of 'IndexOutofStock' in multiple files to ensure consistency in preference handling.
- Integrated the new `productFilter` module in `algoliaLocalizedProduct.js` to handle variant filtering - Replaced direct inventory and stock threshold checks with a centralized `isIncludeOutOfStock` method - Simplified variant selection logic by leveraging the new product filter functionality
…exOutOfStock preference - Added new test case for master-level indexing with IndexOutOfStock=true and product in stock - Updated existing test case for master-level indexing with IndexOutOfStock=true and product out of stock - Expanded test coverage to verify different scenarios of product indexing based on stock availability
- Extended the page load timeout in Cypress configuration from the default to 15 seconds - Helps prevent potential test failures due to slow page rendering or network issues
- Increased page load timeout from 15 to 30 seconds for more stable testing - Modified test:frontend script to run Cypress with Chrome browser in headed mode - Removed redundant cypress:run script
- Added system dependencies for Chrome browser testing - Switched from Electron to Chrome browser in GitHub Actions - Configured Cypress to run in headed mode for more detailed test output
- Improved code formatting and removed redundant syntax
- Removed explicit page load timeout setting in Cypress configuration
* If IndexOutOfStock is false, we only return `true` here if the product has | ||
* ATS >= InStockThreshold. If IndexOutOfStock is true, then always return `true`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very complicated. Again, you must try to come with functions scoped and easy to understand and maintain.
Also, it's always better to write pure functions, i.e. that doesn't change behaviour due to external factor (here, the preferences)
To me the good thing to do here is:
- Have a simple
isInStock(product, inStockThreshold)
function that tells you if the product is in stock - Do this check of in the jobs:
if (!productFilter.isInStock(variant, ALGOLIA_IN_STOCK_THRESHOLD) && !INDEX_OUT_OF_STOCK) { continue; }
var indexName = algoliaData.calculateIndexName('products', locale); | ||
algoliaOperations.push(new jobHelper.AlgoliaOperation(deleteIndexingOperation, { objectID: cpObj.productID }, indexName)); | ||
let localeIndexName = algoliaData.calculateIndexName('products', siteLocales[l]); | ||
algoliaOperations.push(new jobHelper.AlgoliaOperation(deleteIndexingOperation, { objectID: cpObj.productID }, localeIndexName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ok, but this part of the code is only run when the job is in variant-level mode AND there are no attributes computed from the base product.
When there are attributes computed from the base product such as colorVariations
, we don't pass here, we enter in the if (paramRecordModel === MASTER_LEVEL || attributesComputedFromBaseProduct.length > 0) {
condition above.
So the logic needs to be updated above too.
- Refactored `productFilter.js` to introduce a more robust `isInStock` method - Updated multiple indexing scripts to use the new stock filtering approach - Simplified stock threshold and out-of-stock indexing logic - Enhanced test coverage for variant-level indexing scenarios - Removed deprecated `isIncludeOutOfStock` method
- Updated `jobHelper.js` to remove stock filtering from variant record generation - Modified `algoliaProductDeltaIndex.js` to handle out-of-stock product indexing - Updated `algoliaProductIndex.js` to skip out-of-stock product records based on configuration - Improved indexing logic to respect INDEX_OUT_OF_STOCK setting for both master and variant products
…ation - Cleaned up unnecessary imports in `jobHelper.js` - Removed references to stock threshold and out-of-stock indexing preferences - Simplified variant record generation function by eliminating unused dependencies
- Reintroduced page load timeout configuration in Cypress config - Set timeout to 15 seconds to improve test stability for sandbox environment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more time, please explain what scenarios you've tested.
I ran the most simple scenario possible:
- Update a product
- Run the delta job
And I encountered a critical issue.
This is not acceptable.
}); | ||
processedVariantsToSend = localizedMaster.variants ? localizedMaster.variants.length : 0; | ||
algoliaOperations.push(new jobHelper.AlgoliaOperation(baseIndexingOperation, localizedMaster, indexName)); | ||
let inStock = productFilter.isInStock(product, ALGOLIA_IN_STOCK_THRESHOLD); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't work! As I explained in #218 (comment) your filtering function differs from the logic in algoliaLocalizedProduct.js
You had improved the logic yourself in #217 to take into account master products!
For master products, your isInStock
function ALWAYS returns false
The consequence is that you ALWAYS end up in the else
condition where indexName
is not defined...
So here is what you would have seen if you had just run the delta job one time:
ERROR CustomJobThread|1976382993|AlgoliaProductDeltaIndex_v2|algoliaProductDeltaIndex
{"message":"No indexName found for this batch request near line:1 column:70","status":400}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really sorry for the back-and-forths and taking your time unnecessarily with these type errors. I will be more careful before requesting your review, and for each PR, I will create detailed test scenarios. Hopefully, it is working now. I also added my test scenarios; there is only two scenarios (similar) that we can think about, but I believe we can ignore them.
Please let me know what you think.
- Updated `productFilter.js` to handle stock availability for master and variant products - Simplified stock availability checks in `algoliaLocalizedProduct.js` - Refactored product delta indexing to optimize out-of-stock product handling - Ensured consistent stock availability filtering across indexing processes
…xing - Removed unnecessary availability model retrieval in `algoliaLocalizedProduct.js`
- Added timeout for modal backdrop check in Algolia search test - Ensures modal is not present before proceeding with test steps
- Updated `productFilter.js` to improve stock availability checks - Modified `jobHelper.js` to filter out variants based on product inclusion criteria - Adjusted `algoliaProductDeltaIndex.js` to handle index name calculation for localized products - Simplified stock availability logic across product indexing processes
What is Changed
libnss3
,libgbm1
,xvfb
are added for Chromium/Electron in headless environments. Without them, Cypress tests in Electron or Chrome can fail with “MESA: error: … (VK_ERROR_INCOMPATIBLE_DRIVER)” or “glx: failed to create X screen” errors.How to Test
Here are my test scenarios:
Test Master Product: 11736753M
Test Variant Product: 883360541075M , 883360541105M
⬇️ Variant out of stock now
⬆️ Variant in stock now
⏬ both Variants (so master) are out of stock now
Please also consider this PR regarding moving "In_stock" in the master level.
https://github.com/algolia/doc/pull/9652
Doc PR will be created separately after merge